feat: new function set_content_image(), deprecate old set_image_* functions
#303
Conversation
R/deploy.R
Outdated
| stop("Could not locate image at `path`") | ||
| } | ||
|
|
||
| guid <- content$get_content()$guid | ||
| con <- content$get_connect() | ||
|
|
||
| res <- con$POST( | ||
| path = unversioned_url("applications", guid, "image"), |
There was a problem hiding this comment.
See above, we should try the new v1 location, and if that returns 404, then POST to here.
|
Added first pass at supporting the v1 URLs. The exact conditional flow differs between different uses, because some of the calls include parsing the Will want to consider how to generalize this. |
|
Back from vacation and assessing the scope of this PR, I'm going to hold off on implementing changes to the |
tests/integrated/test-thumbnail.R
Outdated
| @@ -0,0 +1,88 @@ | |||
| test_that("set_thumbnail works with local images", { | |||
| scoped_experimental_silence() | |||
| img_path <- rprojroot::find_package_root_file("tests/testthat/examples/logo.png") | |||
There was a problem hiding this comment.
| img_path <- rprojroot::find_package_root_file("tests/testthat/examples/logo.png") | |
| img_path <- "examples/logo.png" |
Does this not work?
There was a problem hiding this comment.
Also, if you put this outside of a test_that block, you should be able to just refer to it in each one rather than having to set it each time.
There was a problem hiding this comment.
Like I replied to the other comment, I haven't futzed with these tests; they are legacy tests adapted to use the new functions.
There was a problem hiding this comment.
That to say — you're probably right, and I should check!
There was a problem hiding this comment.
This comment was in relation to old integration tests applied verbatim to new thumbnail functions. The integration tests need a more thorough look in general outside of this PR. Jon and I discussed this in an off-GitHub conversation at some point. I'm resolving this conversation to indicate that.
|
Something is broken in the integration tests. I was not running those as I was writing these changes. |
Co-authored-by: Neal Richardson <[email protected]>
- handle different types of 404 errors gracefully (depending on connect error code) - be smarter about file extensions
| # User-specified path (wrong extension) | ||
| user_path <- tempfile("thumbnail_", fileext = ".png") | ||
| received_path <- get_thumbnail(item, user_path) | ||
| received <- readBin(received_path, "raw", n = 8) | ||
| expected <- as.raw(c(0x4e, 0x41)) | ||
| expect_equal(paste0(user_path, ".jpeg"), received_path) | ||
| expect_equal(received, expected) |
There was a problem hiding this comment.
This results in thumbnail_blah.png.jpeg is that what we want?
There was a problem hiding this comment.
It seems reasonable to me. The converse (substituting rather than appending the correct extension) also seems reasonable.
This only occurs when the user has provided a file path with the incorrect extension. I could see an argument that in this case, it's better to preserve the entire original user-supplied path and just append the correct extension. That's what @nealrichardson did in his suggestion, so I assumed that it was done with that intent.
There was a problem hiding this comment.
Right. IMO I don't think it's worth trying to be even more clever with the filenames.
Fixes #294
Fixes #304
set_image_path(content, path),set_image_url(content, url),set_image_webshot(content).set_content_image(content, path). The new function can handle a local file path or a remote URL. If the path points to a local file that exists, it will use that. Otherwise, if it is anhttpsorhttpURL, it will attempt to download that image to a temporary file and use that (the old behavior ofset_image_url().In a future PR to the
connectrepo,set_image_webshot()will become a recipe in the SDK Cookbook.Note: this wasn't based off the 0.3.0 release, so the NEWS will have a merge conflict once that PR has merged. Not sure why I started it from
main, but I did 🤷🏻.Replaces #302, which I accidentally closed with a force push.